-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Replaces set_carb_setting and get_carb_setting with direct carb calls
#3922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Replaces wrapper functions set_carb_setting and get_carb_setting from isaacsim.core.utils.carb with direct carb API calls to remove the dependency. The changes systematically replace the generic wrapper with type-specific methods (set_bool, set_int, set_float, set_string) and direct get() calls.
Key Changes:
- Removed imports of
set_carb_settingandget_carb_settingfrom 8 files - Added explicit type checking using
isinstance()to determine which type-specific setter to use - Replaced
get_carb_setting(settings, key)withsettings.get(key) - Type checking order is correct (bool before int, since bool is a subclass of int in Python)
Issue Found:
- Two additional files still use the deprecated imports but were not updated in this PR:
source/isaaclab/isaaclab/sim/utils.py(usesget_carb_setting)scripts/tools/check_instanceable.py(usesset_carb_setting)
Confidence Score: 3/5
- Safe to merge but incomplete - two files still use the deprecated imports
- The refactoring is technically sound with proper type checking and correct implementation. However, the migration is incomplete as two files still import and use the deprecated carb utility functions that this PR aims to remove. This creates an inconsistent state where some code uses the new approach while other code still depends on the old utilities.
- Pay attention to
source/isaaclab/isaaclab/sim/utils.pyandscripts/tools/check_instanceable.pywhich still use deprecated imports
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/sim/simulation_context.py | 4/5 | Replaced carb utils with direct API calls and added type-specific setter methods with proper type checking order |
| source/isaaclab/isaaclab/app/app_launcher.py | 5/5 | Simple replacement of set_carb_setting with type-specific carb API calls for rendering and animation recording |
Sequence Diagram
sequenceDiagram
participant App as AppLauncher
participant Sim as SimulationContext
participant Carb as carb.settings
Note over App,Carb: Before: Using wrapper functions
App->>Carb: set_carb_setting(settings, key, value)
Note over Carb: Type inference in wrapper
Carb-->>App: Setting applied
Note over App,Carb: After: Direct API calls
App->>App: Check isinstance(value, bool/int/float/str)
App->>Carb: settings.set_bool/set_int/set_float/set_string(key, value)
Carb-->>App: Setting applied
Sim->>Sim: _apply_render_settings_from_cfg()
Sim->>Sim: Type check preset values
Sim->>Carb: settings.set_bool/set_int/set_float/set_string(key, value)
Carb-->>Sim: Setting applied
Note over Sim: Uses settings.get() instead of get_carb_setting()
Sim->>Carb: settings.get(key)
Carb-->>Sim: Return value
8 files reviewed, 1 comment
| import isaacsim.core.utils.stage as stage_utils | ||
| import omni.log | ||
| import omni.physx | ||
| import omni.usd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Two files still import and use the deprecated carb utils but weren't updated:
source/isaaclab/isaaclab/sim/utils.py:23imports and usesget_carb_settingon line 1075scripts/tools/check_instanceable.py:70imports and usesset_carb_settingon line 99
These should be updated to use direct carb API calls for consistency.
| ) | ||
| key = rendering_setting_name_mapping[key] | ||
| set_carb_setting(self.carb_settings, key, value) | ||
| if isinstance(value, bool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In simcontext, we have a function called set_setting you can just put this logic there and later only call self.set_setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Replaced deprecated set_carb_setting and get_carb_setting utility functions with direct carb API calls throughout the codebase.
- Removed dependency on
isaacsim.core.utils.carb - Updated all call sites to use type-specific carb methods (
set_bool,set_int,set_float,set_string,get) - Added type detection logic in
simulation_context.pyto correctly route dynamic values to appropriate setter methods - Type checking correctly handles bool before int (since bool is subclass of int in Python)
Confidence Score: 4/5
- Safe to merge after addressing two remaining files that still use deprecated imports
- The changes are straightforward and correctly implemented, but the PR is incomplete as two files (
source/isaaclab/isaaclab/sim/utils.pyandscripts/tools/check_instanceable.py) still import and use the deprecated carb utility functions. These must be updated for consistency. source/isaaclab/isaaclab/sim/utils.pyandscripts/tools/check_instanceable.pystill use deprecated functions and must be updated
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/app/app_launcher.py | 5/5 | Removed deprecated carb utility imports and replaced with direct carb API calls using appropriate type-specific methods (set_string, set_bool, set_float). |
| source/isaaclab/isaaclab/sim/simulation_context.py | 5/5 | Replaced deprecated carb utility functions with direct carb API calls. Added type checking logic to route values to appropriate set_bool/set_int/set_float/set_string methods. Boolean check correctly precedes int check. |
Sequence Diagram
sequenceDiagram
participant App as AppLauncher/SimulationContext
participant Old as isaacsim.core.utils.carb
participant Carb as carb.settings
Note over App,Carb: Before (Deprecated Pattern)
App->>Old: set_carb_setting(settings, key, value)
Old->>Carb: Auto-detect type & call appropriate setter
Note over App,Carb: After (Direct API Pattern)
App->>App: Type check (bool/int/float/str)
App->>Carb: set_bool(key, value)
App->>Carb: set_int(key, value)
App->>Carb: set_float(key, value)
App->>Carb: set_string(key, value)
Note over App,Carb: Getter Pattern
App->>Carb: get(key)
Carb-->>App: value
8 files reviewed, no comments
Description
Removes dependency on
Replaces
set_carb_setting(carb_settings, <setting-name>, <setting-value>with direct carb callcarb_settings.set_string(<setting-name>, <setting-value>),...set_int...,...set_bool...,...set_float.... And replacesget_carb_settingwithcarb_settings.get()Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there